Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Instruments] Escaping issue sub-optimal - Redirect on save success to load defaults from database #7776

Merged
merged 1 commit into from
Nov 17, 2021
Merged

[Instruments] Escaping issue sub-optimal - Redirect on save success to load defaults from database #7776

merged 1 commit into from
Nov 17, 2021

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Nov 4, 2021 •
edited
Loading

Brief summary of changes

This PR fixes the escaping issue that occurs when a text field in an instrument contains an HTML special character. The solution employed here is to redirect on successful save to reload data directly from the database instead of loading it using the _POST data.

Cons:

  • This solution is sub-optimal because the escaping issue will still occur when an error is detected on instrument save and the values MUST be reloaded from the _POST array where the reload can not occur to avoid losing the unsaved data.

Pros:

  • relatively minor changes
  • easy to test

Alternate to: #7777
Fixes #7489
Replaces #7490

Testing instructions (if applicable)

  1. Go to an instrument with text element fields (not including text area element since the issue did not apply to those elements)
  2. Outside of this PR, attempt to save a value in the text element that has quotations in it.
  3. Notice that it does not display properly after clicking "save"
  4. check out this PR
  5. Attempt again to save a value with quotations in it

Make sure to test other usecases as well

  1. behaviour when the form contains an error
  2. behaviour when there are multiple ",<,> or & in the string
  3. behaviour when the instrument status sidebar is modified (is it affected by the reload)

Link(s) to related issue(s)

  • Resolves # (Reference the issue this fixes, if any.)

@ridz1208 ridz1208 mentioned this pull request Nov 4, 2021
Closed
33 tasks
@ridz1208 ridz1208 added 24.0.0-bugs Issues or bug fix PRs that were raised during the testing of release 24.0.0 Category: Bug PR or issue that aims to report or fix a bug Critical to release PR or issue is key for the release to which it has been assigned State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed State: Needs changelog update PR that needs an update in the CHANGELOG file to proceed Category: Security PR or issue that aims to improve security labels Nov 4, 2021
@ridz1208 ridz1208 self-assigned this Nov 10, 2021
@ridz1208 ridz1208 removed State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed State: Needs changelog update PR that needs an update in the CHANGELOG file to proceed labels Nov 10, 2021
@ridz1208
Copy link
Collaborator Author

@driusan on second thought, I don't see why this would need a changelog update... the other fix does, not this one

@ridz1208 ridz1208 assigned driusan and unassigned ridz1208 Nov 10, 2021
driusan
driusan reviewed Nov 10, 2021
View reviewed changes
php/libraries/NDB_BVL_Instrument.class.inc
'&candID=' . urlencode($candID) .
'&sessionID=' . urlencode($sessionID);
return (new \LORIS\Http\Response())
->withHeader("Location", $url);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response should also have a 303 See Other response code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what other response code ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other one. The code's name is See Other.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 2846 above in the same file has the exact same code, should we move this to a function ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2846 already had the 303? I don't see why it would be moved to a function.. setting the response code is always going to be context specific

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont like code duplication

@ridz1208 ridz1208 force-pushed the instrument_coding_fake_fix branch from 3990912 to 8613bc0 Compare November 10, 2021 18:57
@ridz1208
Copy link
Collaborator Author

@driusan urs again

@CamilleBeau
Copy link
Contributor

Saved properly with all cases except when there was a validation error as you mentioned. Coupled with a validation error, the following string ended up partly displaying outside of the textbox:

image
image

@ridz1208
Copy link
Collaborator Author

@CamilleBeau please add passed manual test

@CamilleBeau CamilleBeau added the Passed manual tests PR has been successfully tested by at least one peer label Nov 16, 2021
@driusan driusan merged commit 98206b6 into aces:main Nov 17, 2021
@ridz1208 ridz1208 added this to the 24.0.0 milestone Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan left review comments

@CamilleBeau CamilleBeau CamilleBeau approved these changes

Assignees

@driusan driusan

@CamilleBeau CamilleBeau

Labels
24.0.0-bugs Issues or bug fix PRs that were raised during the testing of release 24.0.0 Category: Bug PR or issue that aims to report or fix a bug Category: Security PR or issue that aims to improve security Critical to release PR or issue is key for the release to which it has been assigned Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Milestone
24.0.0
Development

Successfully merging this pull request may close these issues.

[LorisForm] HTML Text Element does not display quotes
3 participants
@ridz1208 @CamilleBeau @driusan